Add very basic support for command line only applications #1301

geoand · 2019-03-07T13:10:38Z

Relates to: #284

This is probably wrong on a few counts since it touches core things that I'm not entirely familiar with.
I'm opening this however with the hope to get some feedback on the approach.

The basic thing the user would use is the CommandLineRunner, similar to what Spring Boot has.

I also plan on looking into integrating Aesh as mentioned by @stalep

Supersedes #1238

geoand · 2019-03-07T18:50:13Z

@stalep I have a first working example using Æsh here: https://github.com/geoand/quarkus/tree/clrunner%2Baesh (uses Æsh 2.0.0 built locally)

dmlloyd

With some discussion it was agreed that having CDI supply the main method arguments is a possible approach. What is left is a way to gracefully exit the application; this PR uses the approach of adding a new bytecode recorder when the extension is present to call a main method and exit.

Some alternative possibilities:

A config property that triggers immediate exit
Some annotation that can go on to a method which accepts a String[] and returns an int (so the user can return an exit code)
An alternative non-blocking variation on System.exit(int)

dmlloyd · 2019-03-08T09:50:42Z

core/deployment/src/main/java/io/quarkus/deployment/annotations/ExecutionTime.java

+    /**
+     * The bytecode is run from a main method after all RUNTIME_INIT
+     */
+    AFTER_STARTUP


We don't want to use an entire separate bytecode recorder just for this one case.

I just remembered another reason why I needed this recorder - besides needing to have a way to exit. Basically the command line entry points need to be run after everything else (I am thinking basically of CDI/Arc events here).
I didn't see a way to order the execution of templates (which basically would boil down to controlling the order that calls to the templates are written to in the generated class file) - did I miss something perhaps?

Also, in the command line application case, it doesn't make much sense I think to show the startup time after the actual work that is intended has been performed.

So in my view the AFTER_STARTUP execution time makes sense for such applications. Is there a large performance penalty at build time of having another bytecode recorder?

It's not a large penalty by itself, but we don't want to introduce a cost if we don't need to. Since there can only ever be one of these (logically), it probably doesn't even need to be a bytecode recorder. You could for example produce a build item that contains the method info (maybe just the descriptor) to call, and then just emit a call to that method at the right spot if the build item is present.

Got it! I will look into it, thanks!

I'll give this a go to ensure that it can be implemented without too much trouble but I think there is another problem that will arise if we don't have the extra bytecode recorder.
Since we won't be able to capture arguments, we won't be able to support use cases like Aesh, were the user can write multiple commands and it would be our job to feed Aesh with the class names.

So in a non Aesh use case, yes there can only be one entry point (that makes absolute sense since it would be the equivalent of a main method) and no bytecode recorder is needed, but if we want to bring Aesh into the mix, not having one will probably be a problem.
WDYT?

I pushed a commit that removes the need for a new BytecodeRecorder. But it does mean that now some bytecode needs to be "written" by the extension :)

I believe that following a similar approach (where the extension actually participates in the creation of the bytecode) we could support Aesh as well

geoand · 2019-03-08T10:06:44Z

With some discussion it was agreed that having CDI supply the main method arguments is a possible approach. What is left is a way to gracefully exit the application; this PR uses the approach of adding a new bytecode recorder when the extension is present to call a main method and exit.

Some alternative possibilities:

A config property that triggers immediate exit

Some annotation that can go on to a method which accepts a String[] and returns an int (so the user can return an exit code)

An alternative non-blocking variation on System.exit(int)

For CDI injection of the main args, I guess changes would need to be made to Arc?

Of the alternatives proposed for the exit strategy, I personally feel that the dedicated annotation is more natural. In that case could the rest of the infra put in place here be used? I mean that the generated bytecode would invoke the CLI method and then attempt to shutdown in a similar manner as is done here?

geoand · 2019-03-08T10:20:56Z

With some discussion it was agreed that having CDI supply the main method arguments is a possible approach. What is left is a way to gracefully exit the application; this PR uses the approach of adding a new bytecode recorder when the extension is present to call a main method and exit.
Some alternative possibilities:

A config property that triggers immediate exit

Some annotation that can go on to a method which accepts a String[] and returns an int (so the user can return an exit code)

An alternative non-blocking variation on System.exit(int)

For CDI injection of the main args, I guess changes would need to be made to Arc?

Of the alternatives proposed for the exit strategy, I personally feel that the dedicated annotation is more natural. In that case could the rest of the infra put in place here be used? I mean that the generated bytecode would invoke the CLI method and then attempt to shutdown in a similar manner as is done here?

Also if the only way to inject args is via Arc, then I don't we will be able to incorporate Æsh

dmlloyd · 2019-03-08T10:38:02Z

For CDI injection of the main args, I guess changes would need to be made to Arc?

I don't think so (I'm not 100% sure though). The main class would have to expose the args somehow. But it's possible that the main method build step runs too late to produce a CDI bean build item.

Of the alternatives proposed for the exit strategy, I personally feel that the dedicated annotation is more natural. In that case could the rest of the infra put in place here be used? I mean that the generated bytecode would invoke the CLI method and then attempt to shutdown in a similar manner as is done here?

I'm not so sure. We want the user to be able to set the exit status. That's easy from an API perspective but it's difficult from the perspective that the user might be running this in dev mode, where calling System.exit() may have unpleasant effects...

dmlloyd · 2019-03-08T10:38:16Z

Also if the only way to inject args is via Arc, then I don't we will be able to incorporate Æsh

Good point...

geoand · 2019-03-08T10:43:34Z

For CDI injection of the main args, I guess changes would need to be made to Arc?

I don't think so (I'm not 100% sure though). The main class would have to expose the args somehow. But it's possible that the main method build step runs too late to produce a CDI bean build item.

Of the alternatives proposed for the exit strategy, I personally feel that the dedicated annotation is more natural. In that case could the rest of the infra put in place here be used? I mean that the generated bytecode would invoke the CLI method and then attempt to shutdown in a similar manner as is done here?

I'm not so sure. We want the user to be able to set the exit status. That's easy from an API perspective but it's difficult from the perspective that the user might be running this in dev mode, where calling System.exit() may have unpleasant effects...

Does dev mode make sense for these types of applications?

dmlloyd · 2019-03-08T11:00:14Z

Does dev mode make sense for these types of applications?

Perhaps not; I wonder if we can/should detect the case and give an error?

geoand · 2019-03-08T11:07:13Z

Perhaps not; I wonder if we can/should detect the case and give an error?

I was thinking the same thing.
I mean if we have an annotation that will be used to detect the method that the user intends to be used as an entry point to the command line invocation, then I would guess that we could fail when detecting such annotation and when the LaunchMode is Dev.
WDTY?

dmlloyd · 2019-03-08T11:17:22Z

Seems possible, want to give it a try?

geoand · 2019-03-08T11:18:55Z

@dmlloyd Sure thing, I can't get it done today because of regular team work, but I'll try to sneak in some time over the weekend 😉

geoand · 2019-03-08T21:44:02Z

I added a commit that makes dev mode fail when a command line application is used

geoand · 2019-03-11T19:45:17Z

@dmlloyd Do you think that the current form of the PR makes sense as something to build on? I am thinking that if it is, I can also rebase the Aesh work I did previously in another branch.

dmlloyd · 2019-03-11T19:52:50Z

I have a few doubts. Requiring an entire extension and everything else for this seems like way too much. It would be so much better if there was just a small extra bit of code to detect an annotation and then trigger the shutdown behavior if present, skipping the extra ApplicationImpl method and all the other extra generated bytecode.

geoand · 2019-03-11T20:04:03Z

I understand the concern, indeed.
However the advantage I see with the current approach is that it can support various use cases (like the command line runner being a bean, the Aesh approach were the user would write multiple command objects).
I will of course proceed however you see fit since you have much more experience in such matters 😉

geoand · 2019-03-12T13:27:22Z

@dmlloyd would you like me to add a poc of how Aesh could be added using this approach - maybe it will help show the benefits :P

dmlloyd · 2019-03-12T13:53:27Z

Sure, maybe there's something I'm not seeing about how Aesh operates.

geoand · 2019-03-12T13:54:45Z

@dmlloyd cool, I'll try to squeeze it in tonight then

geoand · 2019-03-12T21:21:49Z

@dmlloyd I added another extension that shows how Aesh could be utilized. It's pretty rudimentary, but it hopefully showcases the power of the current approach :)

It would definitely require some clean up, but I just wanted to show what's possible for the time being

Relates to: quarkusio#284

stalep · 2019-03-13T22:54:46Z

the æsh part looks good i think. im planning on doing a pr tomorrow that will upgrade the cli to use æsh 2.0/2.1 as well so there wont be any version difference between devtools/aesh and this.

the idea of using æsh here is that the command line arguments are automatically verified and injected into the command. also, we can give the user access to a QuarkusContext object through the CommandInvocation which adds some fairly unique possibilities to access parts of quarkus without exposing any other api.

geoand · 2019-03-15T15:21:43Z

@dmlloyd Have you had a chance to look at how Aesh could benefit from the approach proposed in this PR?

dmlloyd · 2019-03-18T13:22:45Z

We should talk of this in terms of requirements, which is the "hidden" step between use case and implementation. What behaviors are required for adequate Aesh integration? This would be in addition to the basic stuff:

Allow a program to declare that it exits immediately
Provide a means of controlling the process exit code for immediate-exit programs
Provide a means of consuming command line arguments (perhaps multiple times?)

geoand · 2019-03-18T13:25:12Z

@dmlloyd 👍

Should we setup a call perhaps with @stalep so he can give us his extensive Aesh perspective? My perspective is simply one of a Spring Boot developer used to consuming CommandLineRunner.

dmlloyd · 2019-03-18T13:27:03Z

Sure, a call sounds good. I think the Spring Boot perspective is valid also; we just want to capture any desirable aspects of that experience as usability requirements as well. Once all the requirements are in one place, implementations are easy to evaluate (and sometimes you discover new requirements in this process).

geoand · 2019-03-18T13:28:37Z

Sounds good. @dmlloyd @stalep Any preferences for when to schedule or should I just look at your calendars and find an empty slot? :)

geoand · 2019-04-10T07:00:13Z

I will close this for now. Once we have a consensus on the general strategy to be followed, I can work on it again if you like

geoand mentioned this pull request Mar 7, 2019

Add very basic support for command line only applications #1238

Closed

dmlloyd requested changes Mar 8, 2019

View reviewed changes

geoand force-pushed the cl-runner branch from 79814e9 to 49ad698 Compare March 8, 2019 21:35

geoand force-pushed the cl-runner branch 3 times, most recently from 049cf23 to eb2fc23 Compare March 9, 2019 15:03

geoand force-pushed the cl-runner branch 2 times, most recently from a3fc635 to a2fbce7 Compare March 12, 2019 23:17

geoand added 4 commits March 13, 2019 12:21

Add very basic support for command line only applications

0154727

Relates to: quarkusio#284

Make dev mode fail for command line applications

2edec42

Remove the need for a new BytecodeRecorder

b7a9c2d

Apply minor fix

3f4e099

geoand added 4 commits March 13, 2019 12:21

Add an example of how Aesh application could be used

12ff0a1

Fix broken rebase

b488e52

Fix broken tests

f775609

Use latest released version aesh

e880306

geoand force-pushed the cl-runner branch from 86a3876 to e880306 Compare March 13, 2019 10:21

gsmet added the coding-in-progress label Mar 14, 2019

geoand mentioned this pull request Mar 18, 2019

Add the ability to keep beans from application classes #1545

Closed

geoand closed this Apr 10, 2019

gsmet added the triage/invalid This doesn't seem right label Apr 15, 2019

Add very basic support for command line only applications #1301

Add very basic support for command line only applications #1301

Conversation

geoand commented Mar 7, 2019 • edited Loading

geoand commented Mar 7, 2019 • edited Loading

dmlloyd left a comment

Choose a reason for hiding this comment

dmlloyd Mar 8, 2019

Choose a reason for hiding this comment

geoand Mar 8, 2019

Choose a reason for hiding this comment

geoand Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

geoand Mar 8, 2019

Choose a reason for hiding this comment

dmlloyd Mar 9, 2019

Choose a reason for hiding this comment

geoand Mar 9, 2019

Choose a reason for hiding this comment

geoand Mar 9, 2019 • edited Loading

Choose a reason for hiding this comment

geoand Mar 9, 2019 • edited Loading

Choose a reason for hiding this comment

geoand Mar 9, 2019 • edited Loading

Choose a reason for hiding this comment

geoand commented Mar 8, 2019

geoand commented Mar 8, 2019

dmlloyd commented Mar 8, 2019

dmlloyd commented Mar 8, 2019

geoand commented Mar 8, 2019

dmlloyd commented Mar 8, 2019

geoand commented Mar 8, 2019

dmlloyd commented Mar 8, 2019

geoand commented Mar 8, 2019

geoand commented Mar 8, 2019

geoand commented Mar 11, 2019

dmlloyd commented Mar 11, 2019

geoand commented Mar 11, 2019 • edited Loading

geoand commented Mar 12, 2019

dmlloyd commented Mar 12, 2019

geoand commented Mar 12, 2019

geoand commented Mar 12, 2019 • edited Loading

stalep commented Mar 13, 2019

geoand commented Mar 15, 2019

dmlloyd commented Mar 18, 2019

geoand commented Mar 18, 2019

dmlloyd commented Mar 18, 2019

geoand commented Mar 18, 2019

geoand commented Apr 10, 2019

geoand commented Mar 7, 2019 •

edited

Loading

geoand commented Mar 7, 2019 •

edited

Loading

geoand Mar 8, 2019 •

edited

Loading

geoand Mar 9, 2019 •

edited

Loading

geoand Mar 9, 2019 •

edited

Loading

geoand Mar 9, 2019 •

edited

Loading

geoand commented Mar 11, 2019 •

edited

Loading

geoand commented Mar 12, 2019 •

edited

Loading